-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add subscription-manager fact generation #977
Conversation
This is functional as-is, but could use an integration test and some amount of documentation. How much we put into the docs is up for debate, since this is currently (but could change if deemed necessary) a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This all looks pretty good to me overall; some nits and some test requests but that's it. I'd be OK to merge mostly as is too and doing some things as followups.
Thanks for working on this!!
@@ -747,6 +747,9 @@ pub(crate) async fn rollback(sysroot: &Storage) -> Result<()> { | |||
} else { | |||
println!("Next boot: rollback deployment"); | |||
} | |||
|
|||
sysroot.update_mtime()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this particular changed line but I think we could test this in one of the integration tests pretty easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about punting this to a followup. Unless I'm blind, it doesn't look like in integration we currently test any of upgrade/edit/switch/rollback so that's a whole other thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do in test-image-pushpull-upgrade.nu
for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I was just looking at the stuff under tests-integration
db0a07a
to
e3107a5
Compare
Last TODO before removing draft status is docs for the whole mtime -> .path -> .target -> .service flow |
Also refactor imgstore to base its subpath to be relative to BOOTC_ROOT. This is prep for store::Storage to update the mtime on BOOTC_ROOT. Signed-off-by: John Eckersberg <[email protected]>
This updates the mtime after a successful invocation of upgrade/switch/edit/rollback Signed-off-by: John Eckersberg <[email protected]>
This adds a new systemd path unit which activates on bootc status changing, and in turn triggers a new systemd target. This allows adding arbitrary new systemd services with `WantedBy = bootc-status-updated.target` that will be activated each time the bootc status is updated. Signed-off-by: John Eckersberg <[email protected]>
This adds a new subcommand `bootc internals publish-rhsm-facts` which writes out facts data to /etc/rhsm/facts/bootc.json Signed-off-by: John Eckersberg <[email protected]>
When enabled, this triggers on bootc-status-updated.target to update the facts file for Red Hat Subscription Manager integration via `bootc internals publish-rhsm-facts` Signed-off-by: John Eckersberg <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
Added this, should be good to go now unless we want to hold to get the integration/tmt thing sorted out. |
@@ -56,6 +56,9 @@ static_assertions = { workspace = true } | |||
default = ["install"] | |||
# This feature enables `bootc install`. Disable if you always want to use an external installer. | |||
install = [] | |||
# This featuares enables `bootc internals publish-rhsm-facts` to integrate with | |||
# Red Hat Subscription Manager | |||
rhsm = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK as is, but we will also need to do a corresponding change to the Fedora spec file to turn it on (bcond that is default off in Fedora, on in RHEL?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolves: #929